feat: if condition exists, updating it, otherwise inserting it to the conditions list in the CRD, in the reconcile function#260
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SpaceFace02 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideThis PR changes the reconcile flow to preserve existing status conditions by introducing a generic upsert helper, updates reconcile/tests to assert that foreign controller conditions are preserved and operator-owned conditions are updated in place, and applies several small readability/robustness cleanups across the codebase and test docs. Sequence diagram for reconcile status condition upsert flowsequenceDiagram
participant Reconcile as reconcile
participant Status as TrustedExecutionClusterStatus
participant Upsert as upsert_condition
participant Kube as update_status
Reconcile->>Status: read status.conditions
Status-->>Reconcile: Option<Vec<Condition>>
Reconcile->>Upsert: upsert_condition(conditions, address_condition)
Reconcile->>Upsert: upsert_condition(conditions, uninstall_condition)
Reconcile->>Upsert: upsert_condition(conditions, non_unique_condition)
Reconcile->>Upsert: upsert_condition(conditions, installing_condition)
Reconcile->>Upsert: upsert_condition(conditions, installed_condition)
Upsert-->>Reconcile: conditions updated in place
Reconcile->>Kube: update_status(TrustedExecutionClusterStatus { conditions })
Kube-->>Reconcile: Action::await_change / Action::requeue
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
upsert_conditionhelper currently identifies conditions only bytype_; if there’s any chance multiple controllers share a condition type, you may want to refine the match (e.g., include controller-specific markers or reason) to avoid unintentionally overwriting another controller’s condition of the same type. - In
reconcile,conditionsis cloned solely to build the intermediateinstallingstatus; you could avoid that extra allocation by updating in-place and reusing the sameconditionsvalue for both the installing and installed status updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `upsert_condition` helper currently identifies conditions only by `type_`; if there’s any chance multiple controllers share a condition type, you may want to refine the match (e.g., include controller-specific markers or reason) to avoid unintentionally overwriting another controller’s condition of the same type.
- In `reconcile`, `conditions` is cloned solely to build the intermediate `installing` status; you could avoid that extra allocation by updating in-place and reusing the same `conditions` value for both the installing and installed status updates.
## Individual Comments
### Comment 1
<location path="lib/src/lib.rs" line_range="158-155" />
<code_context>
}
+
+// Update condition if already present, otherwise append(insert) it into the conditions vector.
+pub fn upsert_condition(conditions: &mut Option<Vec<Condition>>, condition: Condition) {
+ let conditions_vec = conditions.get_or_insert_with(Vec::new);
+ if let Some(existing) = conditions_vec.iter_mut().find(|c| c.type_ == condition.type_){
+ *existing = condition;
+ } else {
+ conditions_vec.push(condition);
+ }
+}
\ No newline at end of file
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid overwriting identical conditions to preserve transition semantics and reduce unnecessary status churn.
Because `upsert_condition` always replaces the existing `Condition` when `type_` matches, it resets `last_transition_time` and other fields even when nothing has actually changed. This creates noisy status updates and obscures real transitions. Consider short-circuiting when the incoming condition is effectively the same (e.g., `status`, `reason`, and `message` unchanged), and only updating when there is a meaningful change to preserve transition semantics and avoid unnecessary PATCHes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
SpaceFace02
force-pushed
the
bugfix/upsert-existing-conditions-no-overwrite
branch
from
e883858 to
66267de
Compare
|
I believe we have a setting which makes |
Weird, we have this job that check the cargo and clippy. Might be that the version you have installed is more recent than the toolchain in the CI EDIT: from the CI the version is |
|
@SpaceFace02 thanks, just a couple of nits |
SpaceFace02
force-pushed
the
bugfix/upsert-existing-conditions-no-overwrite
branch
from
78ed44f to
105a3ca
Compare
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
upsert_conditionshould also be used byimage_add_reconcile- Please put fixes by
rustfmtin the same commit
Tip: If you put Fixes: #220 in a commit message, it will auto-link that issue. I've linked it for now.
| let run = if args.cert_path.is_some() && args.key_path.is_some() { | ||
| let config = OpenSSLConfig::from_pem_file(args.cert_path.unwrap(), args.key_path.unwrap()) | ||
| .expect("invalid PEM files"); | ||
| let run = if let (Some(cert_path), Some(key_path)) = (args.cert_path, args.key_path) { |
There was a problem hiding this comment.
(also in response to
I believe we have a setting which makes cargo fmt and clippy return warnings as errors, which made me change a few other parts of the code I did not modify for the feature. Is this intentional?
Weird, we have this job that check the cargo and clippy. Might be that the version you have installed is more recent than the toolchain in the CI
)
I agree with this change, and you're invited to keep making them (in separate commits like you did here), but let me give some context as to why it's not caught by CI in the first place: This has only become a lint recently, and we use a Rust version supported by RHEL/UBI to avoid downstream patching when using a Rust feature that's newer than that. We also use the linting as of that version because sometimes a new Rust feature also becomes a lint to use that feature relatively fast or even in the same version.
With that said, we could update to 1.92 (not for this PR). Ideally it's even possible to get dependabot to update to a version supported by e.g. registry.access.redhat.com/ubi9. This could maybe also go in a doc then.
| Each test can also be run independently using cargo test. Example: | ||
|
|
||
| Note: before running independent tests, make sure TRUSTEE_IMAGE, APPROVED_IMAGE, TEST_IMAGE, RUST_LOG, REGISTRY, TAG env variables are set. Also run `make trusted-cluster-gen` to create the minimal/default boiler-plate yaml manifests for the TEC. |
There was a problem hiding this comment.
good addition, but nit for flow & formatting
| Each test can also be run independently using cargo test. Example: | |
| Note: before running independent tests, make sure TRUSTEE_IMAGE, APPROVED_IMAGE, TEST_IMAGE, RUST_LOG, REGISTRY, TAG env variables are set. Also run `make trusted-cluster-gen` to create the minimal/default boiler-plate yaml manifests for the TEC. | |
| Each test can also be run independently using cargo test. | |
| Before running independent tests, run `make trusted-cluster-gen` to create the default boiler-plate yaml manifests for the TEC and make sure `TRUSTEE_IMAGE`, `APPROVED_IMAGE`, `TEST_IMAGE`, `RUST_LOG`, `REGISTRY`, `TAG` env variables are set. | |
| Example: |
| selector: Some(labels), | ||
| ports: Some(vec![ServicePort { | ||
| name: Some("http".to_string()), | ||
| // Renamed to register-server-port for disambiguation from http, as the presence of a secret indicates http or https. |
There was a problem hiding this comment.
good change, but this comment should be the commit message -- no need to have the code history in the code itself
| } | ||
|
|
||
| // Update condition if already present, otherwise append(insert) it into the conditions vector. | ||
| pub fn upsert_condition(conditions: &mut Option<Vec<Condition>>, condition: Condition) -> bool { |
There was a problem hiding this comment.
I guess this is a bit of a rewrite of https://github.com/kubernetes/apimachinery/blob/a7225b106d19985c2a47654c106cb419007af6d2/pkg/api/meta/conditions.go#L31. Not for this PR, but you could add a TODO to contribute that functionality to kube-rs.
On a different note, this should go to operator/src/lib.rs because you're only using it in-operator.
There was a problem hiding this comment.
That's a great find, yes I'll add the TODO to port it to kube-rs in the future.
And yes, I'll move it to operator/src/lib.rs
| let body = String::from_utf8_lossy(&bytes); | ||
| assert!(body.contains(contains)); | ||
|
|
||
| for (expected_reason, expected_err_msg) in expected { |
There was a problem hiding this comment.
I feel the loop refactor is a little overkill for one use
There was a problem hiding this comment.
Yes, I didn't want to do it initially, but in order to test multiple conditions and to see whether all fields are preserved, I added this.
this method needs ownership of body, as it consumes it, and I didn't want to clone too many times in the caller function, so I thought this is a better way to handle this, albeit a bit verbose.
LMK
| _ => panic!("unexpected: {req:?}"), | ||
| }; | ||
|
|
||
| { |
There was a problem hiding this comment.
this check, and the one below, don't need to be in blocks
There was a problem hiding this comment.
Yes, I had added blocks because there was some import scoping issue of atomic crate, it was being imported in the macro, and since it wasn't scoped initially, it was throwing a double import issue, have fixed it.
| #[tokio::test] | ||
| async fn test_reconcile_uninstall_preserves_foreign_controller_condition_by_inserting_owned_condition() | ||
| { | ||
| let foreign_condition = Condition { |
There was a problem hiding this comment.
but I do think this Condition could be generated from a dummy function
There was a problem hiding this comment.
Yes, good catch, I am using it in 2 places with exact initialization
|
|
||
| // Makes sure that uninstall trigger preserves foreign independent controller conditions, and our operator doesn't overwrite it in the reconcile function. Tests insert of our upsert_condition function. | ||
| #[tokio::test] | ||
| async fn test_reconcile_uninstall_preserves_foreign_controller_condition_by_inserting_owned_condition() |
There was a problem hiding this comment.
Which owned condition? Maybe just test_reconcile_uninstall_preserves_foreign_condition. Same for test_reconcile_install_preserves_foreign_condition_while_updating_owned_condition.
There was a problem hiding this comment.
as these tests belong to operator, this is related to all conditions owned by operator for managing the TEC CRD. In the future, we is possible that we may have certain fields of TEC not owned by operator, and owned by some foreign controller
| "Request body did not contain ForeignCondition, got: {body}" | ||
| ); | ||
|
|
||
| if body.contains(INSTALLED_REASON) { |
There was a problem hiding this comment.
Why is this in a block, don't we always expect that there is exactly one Installed condition?
There was a problem hiding this comment.
Yes, there is only 1 installed condition, but 2 patch calls with different reasons (one with Installing reason and one for InstallationCompleted reason).
Here I am checking that in the last PATCH call (with body containing Reason=InstallationCompleted), does reconcile update existing Installed Condition with new status and reason (expected behaviour), or does it append another condition instead (what we don't want).
SpaceFace02
force-pushed
the
bugfix/upsert-existing-conditions-no-overwrite
branch
from
f1f4e7f to
41e43e4
Compare
Have added the upsert_condition for both image_add_reconcile and ak_reconcile. |
… conditions list in the CRD, in the main reconcile function of operator, attestation key register ak reconcile and reference image_add_reconcile Fix: trusted-execution-clusters#220
…presence of a secret indicates http or https.
SpaceFace02
force-pushed
the
bugfix/upsert-existing-conditions-no-overwrite
branch
from
41e43e4 to
824d2ee
Compare
By adding this to commit message, it will reference the commit instead of the PR right, so once that commit checks in the main branch, will it close the PR and resolve the issue? 
|
3 participants
Bug fix for: #220
Instead of recreating the conditions from scratch each time the reconcile function runs, this makes sure that foreign controller conditions are not overwritten, and same type conditions are updated, rather then recreated; new conditions are inserted.
Summary by Sourcery
Preserve and incrementally update status conditions during reconcile instead of rebuilding them from scratch, while tightening related tests and minor code ergonomics.
Bug Fixes:
Enhancements:
Tests: